Fix getDeclarationModifierFlagsFromSymbolEx for synthetic properties - #63932
Conversation
|
TypeScript Bot (@typescript-bot) test it |
There was a problem hiding this comment.
Pull request overview
Fixes synthetic intersection property accessibility so public constituents are not incorrectly treated as protected.
Changes:
- Prioritizes aggregate accessibility flags for synthetic symbols.
- Adds regression coverage and symbol/type baselines for #63749.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
tsc/internal/checker/utilities.go |
Changes synthetic modifier resolution order. |
tsc/testdata/tests/cases/compiler/syntheticProtectedProperties.ts |
Adds regression scenarios. |
tsc/testdata/baselines/reference/compiler/syntheticProtectedProperties.types |
Records inferred types. |
tsc/testdata/baselines/reference/compiler/syntheticProtectedProperties.symbols |
Records resolved symbols. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
|
Hey Anders Hejlsberg (@ahejlsberg), it looks like the DT test run failed. Please check the log for more details. |
|
Anders Hejlsberg (@ahejlsberg) Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Anders Hejlsberg (@ahejlsberg) Here are the results of running the user tests with tsc comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Everything looks good! |
|
Anders Hejlsberg (@ahejlsberg) Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
|
TypeScript Bot (@typescript-bot) test it |
|
Anders Hejlsberg (@ahejlsberg) Here are the results of running the user tests with tsc comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Everything looks good! |
|
Anders Hejlsberg (@ahejlsberg) Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Hey Anders Hejlsberg (@ahejlsberg), the results of running the DT tests are ready. Everything looks the same! |
|
Anders Hejlsberg (@ahejlsberg) Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (1)
tsc/internal/checker/utilities.go:718
- This branch now bypasses the
ValueDeclarationpath for every synthetic symbol, so it drops non-accessibility modifiers that were previously preserved.createUnionOrIntersectionPropertycan assign a sharedValueDeclarationto a synthetic result (checker.go:21766-21769), andcheckPropertyAccessibilityAtLocationrelies on the returnedAbstractflag to reject abstractsuperaccesses and constructor reads (checker.go:11887-11917). A synthetic property formed from multiple instantiations of the same abstract declaration will therefore be treated as non-abstract. Please merge the synthetic read/write accessibility with the non-accessibility flags from the selected value declaration instead of returning only accessibility andStatic.
if s.CheckFlags&ast.CheckFlagsSynthetic != 0 {
|
There's a suppressed comment but seems plausible? |
I think its very dubious to speculate about abstract properties from unions or intersections that, by definition, don't actually refer to a single class. It's really an implementation attribute, specific to a class, that matters when you inherit from that class. For the same reason, we don't have abstract interfaces. |
This PR corrects and improves checking of set accessor accessibility for properties of unions and intersections of objects. Previously, set accessor accessibility modifiers were effectively ignored in unions and intersections of objects and checking only considered modifiers from get accessors. For example, no errors were reported in the following:
With this PR,
foois now correctly consideredprotectedfor writing, and an error is reported.Fixes #63749.